[wpilibcExamples] Update examples to CommandPtr #5988
Conversation
ncorrea210
changed the title
|
/format |
wpilibcExamples/src/main/cpp/examples/ArmBot/cpp/RobotContainer.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for doing this!
Some comments:
- General (ArmBot, ArmBotOffboard, DriveDistanceOffboard, GyroDriveCommands, others):
CommandPtr(nullptr)shouldn't be valid;Robot.m_autonomousCommandshould default to an empty optional (= "no command") rather thancmd::None()(= "command doing nothing").cmd::None()might be fitting to be returned fromRobotContainer.GetAutoCommand(). - FrisbeeBot: an excellent example of RobotContainer-owned CommandPtrs -- is there a reason to change it?
- GearsBot: an excellent example of RobotContainer-owned command class objects -- is there a reason to change it?
- HatchBotInlined/Traditional: even more than FrisbeeBot/GearsBot, this example showcases choosing between RobotContainer-owned CommandPtrs/command class objects, with detailed documentation about the ownership semantics used. These examples literally are some of the few that excellently solve the problem described by the issue. Why change them?
- MecanumControllerCommand: Other than the comment-and-a-bit there, excellent!
- RamseteCommand: see
optionaldebate in the first bulletpoint. - SelectCommand: as in FrisbeeBot, showcases a good idiom of RobotContainer-owned CommandPtr. Maybe enhance the example to show integration with SendableChooser? (=basically what you wrote in the HatchBot examples).
- SwerveControllerCommand: same as MecanumControllerCommand: other than one small comment, excellent!
Also, well done on removing the unused SendableChoosers!
wpilibcExamples/src/main/cpp/examples/MecanumControllerCommand/cpp/RobotContainer.cpp
Show resolved
Hide resolved
wpilibcExamples/src/main/cpp/examples/MecanumControllerCommand/cpp/RobotContainer.cpp
Outdated
Show resolved
Hide resolved
wpilibcExamples/src/main/cpp/examples/SwerveControllerCommand/cpp/RobotContainer.cpp
Show resolved
Hide resolved
|
It's hard to fully prevent constructing a CommandPtr with nullptr value, since you can pass in a unique_ptr with nullptr value. We could prevent just passing nullptr (e.g. |
|
We can and should test on construction that the passed-in unique_ptr is valid. Does |
|
Yes, because nullptr implicitly converts to unique_ptr. We can check for an invalid unique_ptr on construction and throw? (in addition to deleting the direct-nullptr constructor, which is even better because it is a compile-time error). |
|
I think this CommandPtr issue is beyond the scope of this pull request. I recommend either a draft pull request or a issue for this topic. I mainly say this because I am not 100% sure how to implment this, whether it be something like a static_assert, or some kind of if constexpr expression, or neither. |
|
@Starlight220, about the gearsbot and frisbeebot, are you saying that they should not be changed to CommandPtr? |
|
Correct |
|
I submitted #5991 for the CommandPtr issue. |
|
@Starlight220 I believe I have made all the requested changes. |
|
Just needed to add some quick formatting things to SelectCommand, should all be ready now. |
wpilibcExamples/src/main/cpp/examples/SelectCommand/cpp/RobotContainer.cpp
Outdated
Show resolved
Hide resolved
|
@Starlight220 the requested changes have been completed. |
Starlight220
changed the title
Fixes memory leaks and uses of Command* instead of CommandPtr in GetAutonomousCommand talked about in #5921.